Cleanup QuoteContext #8915
Cleanup QuoteContext #8915
Conversation
nicolasstucki
changed the title
| } | ||
|
|
||
| val args = argsExpr match { | ||
| case Varargs(args) => args | ||
| case _ => qctx.throwError("Expected statically known argument list", argsExpr) | ||
| case _ => Reporting.throwError("Expected statically known argument list", argsExpr) |
There was a problem hiding this comment.
It seems to me the usability is better if methods can be called directly on qctx, as they don't need to remember another top-level name. What's the consideration to introduce Reporting?
There was a problem hiding this comment.
It can be imported if needed. The previous location was unintuitive and a few missed it. Additionally, this seems to be the only reason to not use anonymous QuoteContexts.
There was a problem hiding this comment.
What about using the same trick as Dotty to introduce a method def qctx(using QuoteContext) = ...?
There was a problem hiding this comment.
That would also help. But, still errors and warnings do not belong there.
There was a problem hiding this comment.
I agree with you, logically it is better to separate things.
The little worry I have is that Scala 3 macro authors may also be compiler hackers, compiler plugin writers, and some experience with Scala 2 macros. In all other cases, the error reporting is the same by calling ctx.error. So it would be nice to follow the convention.
There was a problem hiding this comment.
As shown, that can be optionally supported with an implicit conversion.
It also seems that the users of the API did not find the reporting utilities or ended up finding the ones in the reflection API. The second is fine but the overhead could have been avoided by making it more explicit.
nicolasstucki
force-pushed
the
cleanup-quote-context
branch
3 times, most recently
from
2514bd1 to
1576fff
Compare
nicolasstucki
force-pushed
the
cleanup-quote-context
branch
from
1576fff to
d262e99
Compare
| @@ -0,0 +1,35 @@ | |||
| package scala.quoted | |||
|
|
|||
| object Reporting { | |||
There was a problem hiding this comment.
Or, can we make Reporting a trait, and make QuoteContext extend Reporting, the same as it is done in Dotty?
trait Reporting { thisCtx: Context =>
// ...
}For end-users, they don't need to know the concept Reporting, which improves usability.
There was a problem hiding this comment.
That would not help to decouple the concepts. The idea is not to use the QuoteContext directly at all and only let it implicitly track the current scope.
There was a problem hiding this comment.
The idea is not to use the QuoteContext directly at all and only let it implicitly track the current scope.
Thakns for explanation, I can see it is a good design principle. The little question still in my mind is why not use qctx directly if it can improve usability, like it's done in Dotty?
There was a problem hiding this comment.
The problem is that based on the users, the usability of this feature was not great. It may look simple to us because that is the way it is done internally in dotty. I prefer to make it clear for normal users than for users that know the compiler internals.
No description provided.